fix(hir): a later class accessor replaces an earlier one - #9886
fix(hir): a later class accessor replaces an earlier one#9886proggeramlug wants to merge 1 commit into
Conversation
ECMA-262 ClassDefinitionEvaluation installs class elements in source order, so
a second `get x` / `set x` REPLACES the first. `ClassDecl::getters` / `::setters`
are consumed with `iter().find(...)` — first match wins — but every accessor was
appended with `push()`. The shadowed definition therefore stayed live and the
one the program actually defines last was silently dropped.
Every accessor shape is affected, not just getters. Against
`node --experimental-strip-types`, before this change:
instance getter 111 (expected 222)
static + instance getter instance-first (expected instance-last)
static-first (expected static-last)
duplicate setters first:x (expected last:x)
class expression 1 (expected 2)
There is no diagnostic: the program reads a plausible value from the wrong
accessor and keeps running.
Found in Claude-of-Duty, whose `Spring3` pairs an early `set z` (damping) with
a later `get z` (displacement) — legal, if unusual, and it relies on the
read/write asymmetry the spec produces. Perry served the shadowed damping
getter, so `lag.z` and `recPos.z` read 0.46 and 0.42 (their constructors'
damping arguments) instead of displacements. That added +0.88 m to the
first-person viewmodel's Z, moving the rig from 0.3 m in front of the camera
to 0.58 m behind it. All 156 viewmodel nodes then clipped: the overlay pass ran
and issued every draw, and produced no fragments.
`record_class_accessor` overwrites an existing entry instead of appending. The
replacement is keyed on `(name, is_static)`: a static and an instance accessor
of the same name are distinct properties — one on the constructor, one on the
prototype — and collapsing them would trade this bug for another.
Verified: perry-hir 620 passed, perry-codegen 1912 passed. The regression test
covers all four shapes above and fails on each without this change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughClass lowering now replaces duplicate getters and setters by name and staticness. The change applies to declarations and class expressions. A new end-to-end test verifies later-definition behavior for instance, static, private, and class-expression accessors. ChangesClass accessor replacement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes later duplicate class accessors override earlier definitions while preserving distinct static and instance accessors. Regression coverage and successful test suites support merge readiness with no remaining actionable risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hold off merging — this regresses a real workload and I am still diagnosing. Building Claude-of-Duty with this patch fails during world construction: A/B against the same game source, same perry-three, same Bloom: the identical build without this patch boots past that point, and with it fails. So the regression is this change, not the workload. It is not the fix behaving correctly on a duplicate accessor in three.js — I scanned One more unexplained signal: the compiled binary grows from 106 MB to 151 MB with this patch. A change that only drops shadowed accessors should not add code, so lowering is shifting in some broader way I have not accounted for.
|
#9886's last-wins accessor record pushed lower_decl/class_decl.rs to 2050 lines. The member-shape helpers — computed-key naming, the accessor-name survey, and record_class_accessor itself — move to a sibling child module beside the existing class_heritage/member_registration. Unlike the page_meta split, this adds a child rather than renaming the parent, so nothing keyed on the path `lower_decl/class_decl.rs` moves; the two prose references to it elsewhere in the tree stay correct.
|
Landed on |
|
Correction — retracting my previous comment. This patch is not the cause of that regression, and the hold does not apply. My earlier A/B was not controlled. The working build came from Properly controlled now — same base, same game source, only this patch differing:
Both fail, and the binary is byte-identical in size, so neither the failure nor the 106 MB -> 151 MB growth I flagged comes from this change. Both belong to the 108 upstream commits. Notably this patch gets the game past the So the two concerns in my previous comment are both withdrawn:
The patch stands on its original evidence: perry-hir 620 passed, perry-codegen 1912 passed, and the regression test fails on all five accessor shapes without it. Separately, and worth raising on its own: current |
The bug
ECMA-262 ClassDefinitionEvaluation installs class elements in source order, so a second
get x/set xreplaces the first. Perry keeps the first.ClassDecl::getters/::settersare consumed withiter().find(...)— first match wins — but every accessor was appended withpush(). The shadowed definition stays live and the one the program actually defines last is silently dropped.Reproducer
node --experimental-strip-types222 7111 7222 7Note the setter still runs in every case: when a later definition supplies only a getter, the earlier setter stays in force. That asymmetry is the point — see below.
Every accessor shape is affected
Measured against Node on the fixture in the regression test:
111222static-firststatic-lastinstance-firstinstance-lastfirst:xlast:x12There is no diagnostic. The program reads a plausible value from the wrong accessor and keeps running.
How it surfaced
Claude-of-Duty's
Spring3(src/weapons/mathx.js) pairs an earlyset z(damping) with a laterget z(displacement):Unusual, but legal, and it depends on exactly the read/write asymmetry the spec produces. Perry served the shadowed damping getter, so:
Those are added as displacements, contributing +0.880 m to the first-person viewmodel's Z. The rig moved from its authored
hipPos.z = -0.300(in front of the eye) to+0.580(behind it):All 156 viewmodel nodes then sat at camera-space Z ≥ 0 and clipped. The failure was invisible from the renderer's side: the overlay pass ran, the gate passed, all 156
draw_indexedcalls were issued, and the pass's colour reached the screen — with zero fragments.lag.xandlag.ywere correct throughout, because only.zcollides with the damping accessor.The change
record_class_accessoroverwrites an existing entry instead of appending.The replacement is keyed on
(name, is_static), not on the name alone: a static and an instance accessor may legally share a name and are distinct properties — one on the constructor, one on the prototype. Deduping on the key alone would trade this bug for another, which is why the regression test includes a class carrying both.Verification
cargo test --release -p perry-hir— 620 passed, 0 failedcargo test --release -p perry-codegen— 1912 passed, 0 failedduplicate_class_accessor_last_winscovers all five shapes above, with expected output taken from Node. Reverting the one-line lowering change fails it on every shape:Summary by CodeRabbit
Bug Fixes
Tests